New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bundle fund
command
#3390
Add bundle fund
command
#3390
Conversation
Thanks for opening a pull request and helping make RubyGems better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use Github Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Github Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
Just bumping this to hope to get some attention to it. ✌️ |
Overall looks good to me. I have two suggestions:
|
Sure, and individual
From the RFC, I didn't want to do this. Maybe in a future iteration? |
Hey, just bumping this...again. |
@gjtorikian I'm so sorry nobody stepped in to review this before. Thanks so much for being so patient. I'm going to put this on my list for this week. If I haven't reviewed this by next Monday, please ping me again. |
@gjtorikian Could you rebase this PR, it's 1289 commits behind current HEAD, I'd like to make sure it's green before reviewing. I can also do that for you as well if you're busy. |
b25fce4
to
2821205
Compare
@deivid-rodriguez Ok, I've rebased. |
Tests are failing. It's because we removed the |
Ah, ok. Done! |
Hello @gjtorikian! I just read the RFC, it looks really really good ❤️. I agree with everything, including the potentially most controversial parts like not adding an opt-out flag to skip the Regarding the message, what do you think about tweaking it to Regarding the functionality, it looks all good other than that little tweak. I'll go ahead with reviewing the implementation now. |
@deivid-rodriguez That makes complete sense to me, and I've tweaked the message. I also removed the |
e5c684e
to
5f586f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a first pass. Looking good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @deivid-rodriguez ! Everything made sense and I've made the changes.
5dddf03
to
2efc5f3
Compare
So many test issues 🙈! But all good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made another pass, mainly little style issues now.
Ok @gjtorikian, sorry for the back and forth here. The problem is that So, since I don't see an easy fix for this, I propose we go back to #3390 (comment) and address that instead by further tweaking the message to
to make it more clear that any gem exclusions happening before gem installation, for example |
@gjtorikian This looks good to me now! 🎉 Could you rebase and squash the PR into two commits (one for the post_install_message and tests, and one for the addition of |
88fe4f7
to
8e9c31b
Compare
🎆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
I'm aware that at this point I'm probably just being annoying, but technically the commit that needs requested_dependencies
to be made public is the second, so the change making that method public belongs in there 😅.
Other than that, I'd like to thank you so much for your infinite patience and for your polite well-spaced pings along these past months 💜.
5aac6f8
to
9c73af3
Compare
Heh, no worries. It's fixed up! I am just excited to be able to add this and get some visibility for support. I have another change pending for visualizing this metadata on rubygems.org next. 😄 |
Hei, that's awesome to hear. This will make a better ecosystem overall. Regarding my last request, I don't think it's actually fixed because |
It's all good! It's your project, I'm just committing to it. 😆 Re-rebased and pushed. |
Awesome, thanks so much @gjtorikian!! 🎉 |
🥳 |
As a maintainer I know how problematic the next question is but I am compelled to ask: when can we expect a new release with these changes? 🙈 |
We hope to release bundler 2.2.0.rc.2 next week including this feature. |
Add `bundle fund` command (cherry picked from commit ce27b98)
Add `bundle fund` command (cherry picked from commit ce27b98)
Add `bundle fund` command (cherry picked from commit ce27b98)
Add `bundle fund` command (cherry picked from commit ce27b98)
Add `bundle fund` command (cherry picked from commit ce27b98)
Add `bundle fund` command (cherry picked from commit ce27b98)
For more information: - rubygems/rubygems#3390 - rubygems/rubygems#3060
Migrated from rubygems/bundler#7668
What was the end-user or developer problem that led to this PR?
This PR implements the
bundle fund
proposal in this accepted RFC.What is your fix for the problem, implemented in this PR?
The rationale and technical details for the
bundle fund
command are explained in the RFC, but roughly:funding_uri
, can be specified, that points to a funding page for a gembundle install
orbundle update
, a message will be printed out that indicates gems which require support:4 gems you depend on are looking for funding
bundle fund
will list out all those gems, with their funding linksbundle info
will include thefunding_uri
in its printoutbundle fund
to only list information for a specific groupbundle fund
; this is by design